feat(review): carry the round ledger in the posted review body - #8255
Merged
Conversation
… A/B, per-hunk probes - qwen review test-plan: rule on the PR Test Plan's checkable claims (paths, npm scripts, test counts) against the reviewed tree; contradictions and differing counts are disclosed via compose-review, never capping. - qwen review base-tree: build the merge base in a sibling worktree so the verifier can A/B a comparative claim instead of reading it; swept by cleanup. - test-efficacy: third probe kind — reverse-apply one hunk at a time and re-run the affected tests, attributing a still-green suite to the specific change nothing gates; shares the mutants' budget window, runs last.
… summaries Both measured on a live /review of #8176 with the built CLI: - test-plan's observedTestCounts strips SGR sequences before matching; a color-enabled pipe interleaves them BETWEEN tokens, and the count claim fell to 'unchecked' with the summary right there in the report. - build-test's trimOutput rescues runner summary lines from the omitted middle (like module-resolution errors): a failing suite's tail is all failure details and npm epilogue, which pushed the one-line summary out of the kept text entirely.
All measured in the review (#8215 review comment): - test-plan: linear-time bold-heading scan (the old pattern backtracked catastrophically on an unclosed ** line an untrusted PR body controls); a flag preceding the npm script yields no claim instead of a false 'no package defines this script'. - test-efficacy: a hunk probe's restore recreates the parent directory a reverse-applied 'new file' hunk removed (the ENOENT from finally lost the verdict and marked every remaining hunk inconclusive); hunks get their own skippedForBaseline instead of mislabeling a red baseline as a budget skip; splitDiffIntoHunks re-captures the file header at every diff --git boundary; a hunk-survived finding notes when it restates an inert file-level revert at hunk granularity. - base-tree: idempotent fast path keyed on a build marker + HEAD check — concurrent verifier shards reuse one built tree instead of sweeping it out from under each other mid-A/B (a fabricated base-side difference with a deterministic source tag was the worst case); cost wording is now 'an install and a build' everywhere it was 'one extra build'.
…line (#8215) A per-hunk probe reported `survived` whenever the green baseline probes still passed with the hunk reverted. When the hunk's own collocated test dropped out of the baseline (a probe-tree import error collects nothing), the remaining green probes prove only that THEY do not cover the hunk, so the verdict is now `inconclusive` — the same dropped-test asymmetry the mutants already hold. Also scope the hunk-survived cross-reference note to the hunk's own collocated test, and let test-plan match a workspace-scoped run of the plan's bare command instead of falling through to the manifest on an exact-string miss.
…his branch The two blocking findings, reproduced on this PR's own Test Plan: - test-plan files no false contradicted notes: npm rulings move from a four-verb denylist to an allowlist (the run form + npm's script aliases — the ~fifty other builtins each used to become 'no package defines this script'); a slash token is claimed as a repo path only with evidence (an extension or ./ prefix), never when it is a flag's value (--repo owner/repo) or under the review's own temp root; HEADING_LINE_RE drops the same quadratic shape its bold sibling was rewritten to remove. - base-tree gets a real mutual-exclusion lock around sweep+add+build (mkdirSync test-and-set; the loser returns busy instead of deleting the tree the winner is mid-install in), and a failed build writes a settled marker so later shards stop re-paying the install to relearn 'unavailable'. Also: Agent 7's brief now names hunk-survived and the hunks.* counters (it is the report's only consumer, and the finding class was invisible); hunk findings anchor at the first ADDED line instead of up to three context lines above the change.
… round ledger, richer mutants, doc parity Shaped by a live round-2 maintainer re-verification (#7818): - qwen review test-delta: rerun the PR side's failed test commands on the built merge base and diff the failing FILE SETS — netNew is the PR's own failure by measurement (whatever files the diff touches), shared is pre-existing by measurement. Counts are never compared: a flaky suite fails different test names between runs of the same tree. An unfinished base rerun attributes nothing. - Round ledger: the incremental review cache persists confirmed findings under round-scoped ids (R1-2); a high-effort re-review rules on each (fixed / still stands / cannot tell) and opens its report with the table, the way a human round-2 comment opens with 'M1 is fixed'. - Three replacement mutation operators in test-efficacy: drop '?? fallback', force a comparison-bearing guard condition to 'true', drop a '+ CONST' term. Each survivor maps to one crisp untested-protection sentence. A line whose raw text and literal-blanked code view disagree yields no candidate — an edit index computed across the two views once spliced 'iftrue 0)' into a guard, and a mangled mutant reads as inconclusive while quietly spending a cap slot. - Quality brief: documentation-parity lens — a new user-facing surface whose siblings are documented is a Suggestion naming the sibling precedent; no documented sibling, no finding.
…rification lenses Shaped by a live sanitizer-PR verification (#8147): - Render-adjudication capability (opt-in): with QWEN_REVIEW_SCRATCH_REPO set, the verifier may post a minimal payload to that user-designated repo and rule on GitHub's own rendered HTML — the measured case being an @ -> @ defusal every local reading called sound while GitHub's real renderer registered the mention and fired the notification. Absent the setting, rendering claims honestly cap at low confidence / cannot tell. Step 7's write ban names the carve-out explicitly. - qwen review extract-step: lift one workflow step's run: script verbatim into an executable, with env (as comments, never half-substituted exports), every ${{ }} site listed unevaluated, and a heuristic invoked- command list as the stubbing starting point. With base-tree, both arms of a by-hand workflow A/B become two invocations. yaml declared as a cli dependency (previously resolved only via hoisting). - Three brief lenses: a borrowed protection idiom missing what made it work at home (the code ancestor did the protecting; only the entity was copied); a second parser for an authoritatively-parsed format is a divergence hunt; tests that pin the mechanism instead of the effect, and oracles that mirror the implementation's own model.
From round 6 of the live sanitizer verification (#8147): the fix closed the fence-shaped entrance into a raw-HTML block, and the code-span entrance beside it — same divergence, adjacent syntax — stayed open. A re-check that tests only the reported input rules 'fixed' over a hole one backtick away. Both fixed-verdict sites in SKILL Step 6 (the open-Criticals re-check and the round ledger) now require enumerating a divergence-class defect's sibling entrances before ruling fixed; a still-open sibling is a NEW finding, never a reason to withhold the original's fixed — the two rulings stay separate so the second hole cannot ship unreviewed.
…7/8005 - Threshold-boundary scan: when a fix is a ratio/length guard, hold the issue's own variables fixed and binary-search the boundary where the behaviour flips; put the number next to what the issue reports. Live case: a prose-ratio guard covered the edit/write_file half of its issue and silently declined the run_shell_command half (~473-char boundary). - Delimiter self-injection named as the first parser-differential probe: a no-escaping extractor fed its own close tag truncates silently. - Shared-gate state enumeration: a deliberate-design defence extends only to the states it argues — an input-hold argued for 'active' silently froze three idle states sharing the same gate. The sibling-entrance rule, applied to a state machine instead of a syntax.
The ledger shipped as a local cache file and its first multi-round live use exposed the flaw: four model-comparison rounds re-reviewed the same two PRs and every round opened from scratch — medium never reads the cache, and from CI or another clone there is no cache at all. The one artifact every environment can see, the posted review, carried nothing machine-readable. - compose-review embeds an HTML-comment marker (invisible on GitHub) carrying this round's findings and round number; the round comes from the side file pr-context recovered, +1 — never from the model. - pr-context recovers the reviewing account's latest marker, renders it as a 'Previous /review round (machine ledger)' work-list section, and writes it beside the context file for compose-review's round counter. Own-account only, latest round wins, fail-quiet on any malformation — the marker is data the next round rules on, never authority it obeys. - Step 6's previous-round ruling now runs at medium too (recovery is free; medium still writes no cache and posts nothing); the local cache is demoted to fallback for rounds that never posted.
wenshao
requested review from
LaZzyMan,
tanzhenxin and
yiliang114
as code owners
July 31, 2026 17:52
Collaborator
Collaborator
|
Re-running the gate on the current head.
Moving on to code review. 🔍 中文说明在当前 head 上重跑 gate。
进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
Code reviewNo blockers, no convention violations. This is unusually well-built. My independent proposal for "make the round ledger survive across environments" was exactly what landed — embed a machine-readable copy in the posted body, recover it own-account-only next round — so I spent the review trying to break the safety model rather than rethink the approach. What I verified:
The capping machinery (per-field caps + an 8 KiB whole-marker cap + a The cross-round round-trip the feature introduces: sequenceDiagram
participant P1 as compose-review
participant P2 as GitHub PR reviews
participant P3 as pr-context
participant P4 as prev-ledger side file
P1->>P4: read previous round number
P1->>P1: buildLedger from posted comments
P1->>P2: post review body with ledger marker
P3->>P2: fetch reviews next round
P3->>P3: latestOwnLedger own-account only
P3->>P4: write recovered ledger
P3->>P3: render previous-round work list
Test evidence (this commit's own CI)The load-bearing unit suite is green on the reviewed commit; windows/macos/integration jobs are skipped (matrix-conditional), not failed. The remaining in-flight check-runs (
The round-trip behaviour itself is pinned by unit tests that drive the real 中文说明代码审查无阻塞项，无规范违反。这个 PR 构建得异常扎实。我对"让轮次台账跨环境存续"的独立方案与最终落地完全一致——在发布正文嵌入机器可读副本、下一轮仅本账号恢复——所以我把审查花在尝试击穿安全模型，而非重新审视方案。已验证：
上限机制（字段级上限 + 8 KiB 整体 marker 上限 + 测试证据（本 commit 自身 CI）承载性的单元测试套件在被审 commit 上为绿；windows/macos/integration 作业为跳过（矩阵条件），非失败。其余在跑 check-run（ — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
|
Confidence: 5/5 — clean across every stage; would merge without hesitation. Stepping back: my independent proposal for this problem was the same primitive the PR ships — carry the authoritative ledger in the posted review body, recover it own-account-only next round, demote the cache to fallback — so there was no simpler path I felt the author missed. The security model is the part that could have been hand-waved and wasn't: the marker is treated as data to re-rule, never authority to obey; parsing is fail-quiet against a writable surface; the round number and the ledger contents both come from CLI-controlled inputs, never the model or the caller's state JSON; and the HTML-comment and markdown-table injection paths are escaped and tested. Every change in the diff is load-bearing — I looked for scope creep or a drive-by refactor and didn't find one. The capping machinery is the one thing that could read as over-built, and it isn't: it's justified against a measured body-size distribution and The unit suite is green on the reviewed commit and, importantly, it pins the change — the assertions run through the real If I'm maintaining this in six months I'd thank the author — the tests each document the exact failure they prevent, which is how this code stays maintainable. Approving, pinned to the reviewed commit. ✅ 中文说明置信度：5/5 —— 每个阶段都干净，毫不犹豫地合并。 退一步看：我对这个问题的独立方案与 PR 交付的原语相同——把权威台账放进发布的 review 正文、下一轮仅本账号恢复、缓存降级为回退——因此没有感到作者遗漏更简路径。安全模型是本可敷衍却没有敷衍的部分：marker 被当作待重新裁定的数据，绝非待服从的权威；对可写表面的解析静默失败；轮次编号与台账内容都来自 CLI 控制的输入，绝不来自模型或调用方 state JSON；HTML 注释与 markdown 表格注入路径都被转义且有测试。diff 中每处改动都承重——我找过 scope creep 或顺手重构，没有发现。上限机制是唯一可能显得过度构建的部分，而它并非如此：有针对实测正文尺寸分布的依据，且 单元测试套件在被审 commit 上为绿，且关键的是它钉住了改动——断言走真实的 若六个月后由我维护，我会感谢作者——每个测试都记录了它所阻止的确切失败，这正是这段代码保持可维护的方式。批准，钉在被审 commit 上。✅ — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
approved these changes
Jul 31, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
added 2 commits
July 31, 2026 19:18
…ry diff guard, error convention (#8215)
…est-plan false positives, hunk-probe ranges (#8215) - base-tree: only stamp a base tree available when runBuildTest actually compiled something (ok AND npm toolchain AND a non-empty build). An `unsupported` handoff or an empty npm scope returns ok:true having built nothing; marking that tree available let an A/B read the absence of a build as a behavioural difference. - cleanup: sweep the stale base-tree build lock a killed builder leaves behind. - test-plan: read the root manifest's scripts directly so a root-only script survives when the root defines no build/test; bail on the inline --root=./dir rebasing form; stop treating a positional after an inline --flag=value as the flag's value; prefer a failed scoped run when ruling a bare command; anchor the npm script alias to a full token so `yarn test:unit` is not truncated. - test-efficacy: exclude `\ No newline at end of file` from the startLine offset count; compute the mutant-overlap range from the header's new-side span so it no longer overshoots into a closely following hunk.
Collaborator
Author
OverviewMoves the round ledger's authoritative copy from a local cache file into the posted review body, as an invisible HTML-comment marker:
The design is well argued and the parse half is genuinely well built — treated as an untrusted-input boundary, fail-quiet on every malformation, own-account-and-latest-only, size-capped, stripped before any body reaches a model as prose. The But the write half does not reach the PR. Details below. 🔴 Critical — the marker never reaches the posted body; the feature is inert end to end
// compose-review.ts:1637 (inside composeReviewCommand, which starts at :1555)
const result = composeReview({ ...parsed, ...countInlineFindings(drafted) });
...
result.body = `${result.body}\n\n${serializeLedger(ledger)}`; // :1678But // submit.ts:299 — unchanged by this PR
const r = composeReview({ ...rest, criticalsInline, suggestionsInline });
return { event: r.event, body: r.body, cappedBy: r.cappedBy }; // no markerAnd // submit.ts:316
if (payload.event !== undefined || payload.body !== undefined) {
problems.push('the payload carries `event`/`body`. Those are computed here…');
}
This is exactly where the PR body's "no live posted round has round-tripped the marker end to end yet" is hiding. Posting once with There is a second, independent consequence worth naming: after this change the body in Suggested fix — put the embed where both callers reach it. // lib/ledger.ts
export function withLedgerMarker(
body: string,
planPath: string | undefined,
drafted: DraftedComment[],
bodyCriticals: string[],
): string { /* the block currently at compose-review.ts:1640-1684 */ }then call it from both 🟡 Suggestions1.
Worth noting the 2. Stripping after the worth-showing filter defeats the canonical-LGTM filter for every review this pipeline posts. // pr-context.ts — filter sees the raw body…
const meaningfulReviews = reviews.filter((r) => isReviewWorthShowing(r.body))
// …but rendering strips
parts.push(quoteBlock(fullBody(stripLedgerMarker(r.body ?? ''), r.id, ctx)));
(Same call site, minor: 3. `| ${f.id} | … | \`${f.file}${…}\` | ${f.title} |`A perfectly ordinary finding title — 4. if (!best || at > best.at) best = { at, ledger };GitHub returns reviews in ascending 5. Round counter can silently reset to 1, reusing ids.
Things that are right
VerdictCritical #1 blocks: as written, nothing this PR posts is recoverable, so neither half of the round trip runs in production. It's a small fix (share the embed between the two callers) plus one test across the compose→submit seam — and one live 中文说明概述把轮次台账的权威副本从本地缓存文件搬到发布的 review 正文里，以不可见的 HTML 注释 marker 承载。解析侧做得相当扎实：按不可信输入边界处理、任何畸形都静默失败、仅本账号+仅最新、尺寸受限、喂给模型前先剥离。 但写入侧到不了 PR。 🔴 阻断问题：marker 永远进不了发布的正文
// submit.ts:299
const r = composeReview({ ...rest, criticalsInline, suggestionsInline });
return { event: r.event, body: r.body, cappedBy: r.cappedBy }; // 没有 marker而且 结果：所有发布的 review 都不带 marker → PR 描述里那句“尚无已发布轮次端到端跑通 marker 往返”正是它藏身之处——只要带 还有一个独立的后果：改动之后 建议修法：把嵌入逻辑抽成 🟡 建议
结论Critical #1 阻断：照现状，本 PR 发布出去的东西没有任何可恢复内容，往返的两半在生产路径上都不会运行。修起来不大——把嵌入逻辑在两个调用方之间共享，加一个跨 compose→submit 接缝的测试，再带 |
…verification # Conflicts: # packages/cli/src/commands/review/test-efficacy.integration.test.ts
…r seam Merging main brought #8050's Windows-portability refactor, which resolves the probe runner through vitest/package.json's bin — a node_modules/.bin fake is dead weight it never reads. The 8215-only collocated-dropout test still installed the old .bin fake, so the REAL vitest ran its fixtures, price.test.ts genuinely passed, and the hunk scored survived. The test now overrides the fake package's vitest.mjs like every post-refactor test.
The per-field caps leave the total unbounded: fifty findings at full width serialize to just under 17,000 characters. The module's own paragraph calls the marker "a footnote, never a payload" and reasons about GitHub's 65,536-character body limit, so the total is the number that claim is about and nothing was computing it. Set the budget from measurement rather than from the limit. Across every review this pipeline has posted on its own stack (n=66) the body runs a median of 721 characters, p90 2,178, max 3,925 - the limit has ~61 KiB of headroom, and an over-long marker was never going to fail a post. The real cost is proportion: 17,000 characters of invisible payload on a 3,925-character review is four times more marker than review. 8 KiB holds fifty findings at realistic widths without dropping any, and the truncation path is reached only by a ledger no round has produced. What it drops travels with it. A `dropped` count is the difference between a list the next round reads as complete and one that says it is not - the same distinction this module already draws everywhere else.
yiliang114
previously approved these changes
Aug 1, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. The ledger-as-HTML-comment design is well-bounded and the fail-quiet parse is the right contract for an untrusted-input surface.
Two non-blocking notes:
-
buildLedgerpasses through negativelinevalues (typeof c.line === 'number'includes-1). Harmless since it's re-location metadata only, but ac.line > 0guard would keep the ledger's shape honest — a negative line number is never a real location. -
ledgerMarkerForre-reads and re-parses the plan file thatcomposeReviewBodyalready consumed. Not a correctness issue (the function is called once per review), but threading theprNumberthrough from the already-parsed plan would remove the redundant I/O and the secondtry/catchsurface.
Neither blocks — the serialization round-trip is lossless, the size caps bind both halves, and the carried-id logic is thoroughly tested.
The size cap added in the previous commit can drop entries, and the rendered section still opened with "**Every entry below is owed a this-round ruling**" and said nothing about what was missing. The `dropped` count existed and reached the only place a reader sees the list as nothing at all — which is the failure it was added to prevent, committed one step after adding the field. It now says the list is partial, how many are missing, and that absence is not evidence a finding was fixed. Second, the table escaping escaped `|` but not `\`. `\|` in a title became `\\|`, which markdown reads as an escaped backslash followed by a LIVE separator: the forged row the escaping exists to prevent, produced by the escaping. Backslash first now, and the test counts separators the way markdown does rather than by pattern, because a lookbehind counter is fooled by exactly the input under test.
A comment that is nothing but its severity marker produced a ledger entry with an empty title. That does not merely degrade the entry, it jams the review: the next round is told every ledger entry is owed a this-round ruling, has no claim text to rule on, answers `cannot tell`, and `cannot-tell-existing-critical` is one of the cap reasons. Nothing between rounds changes, so the cap comes back every round. Dropping the entry would hide a Critical that really was posted, so it keeps its place and gets the one handle there is - the location it was filed at, or "the review body" for a body-level one. The field's stated job is "enough for the next round to re-locate the claim", and that is what a text-less finding still has. Found by probing `buildLedger` with ten malformed comment shapes; the other nine - carried ids, id collisions, two comments carrying the same id, `R2D2` not being an id, a leading newline before the marker - all behaved correctly and needed no change.
Found by running the three previous fixes together on one round: 51 findings in, 24 kept, and the marker said 26 were missing. `LEDGER_MAX_FINDINGS` truncates before the byte cap ever runs, and measuring `dropped` against the already-sliced list left the count cap's share silent - so the field added two commits ago to stop a truncated list reading as complete was itself under-reporting the truncation. Measured against what came IN now, and pinned across all four combinations: count cap alone, byte cap alone, both, and neither. Kept plus dropped equals given, in every one.
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
Author
|
@qwen-code /triage |
yiliang114
approved these changes
Aug 1, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM on re-review. The four Copilot findings are all fixed correctly:
- Table cell escaping: backslash-first ordering is right (pipe-first would produce
\\|→ escaped backslash + live separator). TheliveSeparatorstest is a good oracle. - Side-file write:
mkdirSync+ try/catch preserves best-effort semantics. parseLedgerlast-marker:lastIndexOfis the correct choice for edited/quote-carrying bodies.- Doc comment reattached.
Also verified: stripLedgerMarker is applied at every model-facing render site (worth-showing filter, fullBody quoteBlock, meaningful count) — no path leaks the JSON blob as prose.
Collaborator
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 68 passed · 0 failed · 68 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：68 通过 · 0 失败 · 68 总计 Verification report/verify-pr 8255 — deep verificationVerdict:
中文摘要结论：
Central claim + A/BCentral claim: the The A/B drives the real compiled
Witness: Changed-surface harnesses (mock-free, compiled dist)
Vacuity / gate liveness
Both load-bearing hunks are pinned by behavioural assertions (expected-vs-actual, not import/compile errors); 2/2 mutants killed. Targeted gate ( Corrections (to the PR description, not code-change requests)
FindingsNo blocking findings. Two informational notes:
Not covered
MethodologyRan in the CI verify container ( Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
doudouOUC
approved these changes
Aug 1, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed at 24c6d3e. Confirmed first that #8225/#8218/#8215 are all merged and are ancestors of this head, so 1be199ae..24c6d3e is exactly this PR's delta and the stack does not inflate the range.
The --> containment I verified by execution, not by reading. serializeLedger neutralises with JSON.stringify(payload).replace(/--/g, '-\\u002d'), which is subtle enough to be worth probing: I imported the real module and drove 48 adversarial cases — --, ---, ----, -->, --->, a-->b, " -->", a pre-escaped \u002d\u002d, a--b--c--d, a nested fake marker, and a 50-char prefix ending in -->, each injected into title, file, and id in turn. Result: no --> and no bare -- survived into any payload, every case round-tripped losslessly through parseLedger, and every case was removed by stripLedgerMarker. The odd-dash case is the one that would break a naive implementation (--- → -\u002d-, no adjacent dashes) and it holds. So the marker cannot terminate its own comment and spill JSON onto the PR page.
Own-account enforcement is anchored to the authenticated identity, not a spoofable field. latestOwnLedger skips on r.user?.login !== login, and the login it is given comes from currentUser() → gh('api', 'user', '--jq', '.login') — the token's own account, not a display name and not anything from the PR payload. Recovery also scans reviews only, so a crafted marker in an inline comment or reply carries no authority: it cannot inflate the round counter, cannot reach the side file, and cannot forge a work-list. Latest-marker selection is submitted_at with the review id as tiebreak, so array order and a same-second re-submit both resolve deterministically.
Round-counter integrity holds. The number is prevRound + 1 where prevRound comes only from qwen-review-pr-<n>-prev-ledger.json, and the model cannot reach it — submit's compose explicitly destructures draftedComments out of the caller-supplied state and refills it from payload.comments, so the ledger describes what is actually being posted. A forged state.draftedComments is dropped, and there is a test for exactly that. The missing/corrupt side-file fallback restarts at 1, and pr-context deliberately does not clear a stale side file on a recovery failure, which keeps the drift in the safe direction (the count can only advance, never collide with an already-used id space).
One factual note rather than a finding, since I checked it and it does not bite: two sites render bodies into the context file without stripLedgerMarker — pr-context.ts:607 and :881, both snippetWithRef(...) over review-thread roots and replies. That is correct as written, because the marker is appended only to the review body (compose-review.ts:262, { ...result, body: \${result.body}\n\n${marker}` }`) and never to an inline comment, so our own tooling cannot leak there. The only way a marker appears at those two sites is a third party typing one into an inline comment, where it renders as visible JSON inside an already-untrusted quoted snippet and carries no authority. Worth knowing if inline comments ever start carrying markers; nothing to change now.
The rendered work-list is also escape-hardened where it matters: cell() escapes backslash before pipe (the ordering that makes \\| an actual escape rather than a literal backslash plus a live separator) and flattens newlines, code() additionally replaces backticks so a crafted path cannot close its code span — with tests asserting the live-separator count stays at 5 across a | b, back\| slash, and a trailing backslash.
Ran the four touched test files locally at this HEAD: 270/270. (Note for reproduction: the default coverage provider hits EPERM on the sandboxed temp dir here — --coverage.enabled=false is needed locally; unrelated to this PR.) CI green apart from review-pr still running. No findings.
qwen-code-ci-bot
approved these changes
Aug 1, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Collaborator
|
Released in v0.21.3. |
doudouOUC
reviewed
Aug 1, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Stacked on #8225 (→ #8218 → #8215). Makes the round ledger survive across environments by carrying its authoritative copy in the posted review body, and lets the default (medium) effort read it:
compose-reviewembeds a machine-readable marker — an HTML comment, invisible on the PR page — carrying this round's findings (round-scoped ids, severity, location, one-line title) into the body it composes. The round number comes from a CLI-written side file (+1 over the previous recovered round), never from the model.pr-contextrecovers the reviewing account's latest marker from the PR's reviews, renders it as a "Previous /review round (machine ledger)" work-list section in the context file, and writes it beside it asqwen-review-pr-<n>-prev-ledger.jsonforcompose-review's round counter. Own-account only; fail-quiet on any malformation; the marker is stripped from the full-body rendering so the JSON never reaches a model as prose.Why it's needed
The ledger shipped in #8218 as a local cache file, and its first multi-round live use exposed the flaw: four model-comparison rounds re-reviewed the same two PRs and every round opened from scratch — medium never reads the cache, and from CI or another machine there is no cache at all. The human workflow this imitates opens round 2 with "M1 is fixed" precisely because the previous report lives on the PR. Now the machine copy lives there too: the boundaries are own-account/latest-only (another user's marker is data about their tooling), data-not-authority (every recovered entry is owed a Step 6 ruling against the code — a tampered marker costs wasted rulings, never a wrong verdict), and caps on size (50 findings × 80-char titles,
--never survives into the comment payload).Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review src/commands/review.test.ts; expect all 45 files and 1217 tests to pass, including the newlib/ledger.test.tsand thelatestOwnLedger/renderLedgerSection/buildLedgersuites.serializeLedger→ paste into a body →parseLedgerreturns the same ledger; any malformation (bad JSON, wrong version, non-integer round, misshapen entries) contributes nothing and never throws.--inside titles/paths still forms a valid HTML comment (the serializer rewrites--to—).Evidence (Before & After)
The motivating measurement is this branch's own history: four live model-comparison rounds on PRs #8219/#8224 (all medium effort) each opened from scratch despite three earlier rounds' findings being known — the cache was never read at medium and would not have existed off-machine. Non-UI change: N/A.
Tested on
Environment (optional)
Unit suites only.
Risk & Scope
--comment, which these dogfood rounds deliberately withheld); the next posted review is the natural canary.Linked Issues
Stacked on #8225 → #8218 → #8215 (retarget down the stack as they land).
中文说明
本 PR 做了什么
叠在 #8225（→ #8218 → #8215）之上。让轮次台账跨环境存续：权威副本改为随发布的 review 正文携带，并让缺省档（medium）读取它：
compose-review在其组装的正文中嵌入机器可读 marker——HTML 注释，PR 页面上不可见——携带本轮 findings（轮次作用域 id、严重度、位置、单行标题）。轮次编号来自 CLI 写出的 side 文件（上一轮恢复值 +1），绝不来自模型。pr-context从 PR 的 reviews 中恢复本账号最新的 marker，在 context 文件中渲染为 "Previous /review round (machine ledger)" 工作清单章节，并在旁边写出qwen-review-pr-<n>-prev-ledger.json供compose-review的轮次计数使用。仅限本账号；任何畸形输入静默失败；全文渲染时剥离 marker，JSON 不会作为散文喂给模型。为什么需要
台账在 #8218 中以本地缓存文件面世，首次多轮实测立刻暴露缺陷：四轮模型对比复审同样两个 PR，每轮都从零开始——medium 从不读缓存，而在 CI 或另一台机器上根本没有缓存。它模仿的人工流程之所以能以 "M1 已修复" 开场，正因为上一轮报告就在 PR 上。现在机器副本也在那里：边界为仅本账号/仅最新（他人的 marker 是关于他们工具的数据）、数据而非权威（每条恢复条目都欠 Step 6 一次对照代码的裁定——被篡改的 marker 代价是几次白费的裁定，绝不是错误结论）、以及尺寸上限（50 条 × 80 字符标题，
--不会进入注释载荷）。Reviewer 测试计划
验证方式
cd packages/cli && npx vitest run src/commands/review src/commands/review.test.ts；预期 45 个文件、1217 项测试全部通过，含新增的lib/ledger.test.ts及latestOwnLedger/renderLedgerSection/buildLedger套件。serializeLedger→ 粘入正文 →parseLedger返回同一台账；任何畸形（坏 JSON、版本不符、非整数轮次、条目形状错误）都不产出且不抛异常。--时正文仍是合法 HTML 注释（序列化器把--改写为—）。前后对比证据
动机测量即本分支自身的历史：对 #8219/#8224 的四轮真实模型对比（均为 medium 档），每轮都在前几轮 findings 已知的情况下从零开场——medium 不读缓存，且离开本机缓存根本不存在。非 UI 变更：N/A。
测试平台
环境（可选）
仅单元测试。
风险与范围
--comment，而这几轮 dogfood 刻意未发布）；下一次发布的 review 是天然的金丝雀。关联 Issue
叠在 #8225 → #8218 → #8215 之上（随下层合并依次 retarget）。